Skip to content

clarify destination rule docs#2428

Merged
istio-testing merged 3 commits intoistio:masterfrom
ramaraochavali:fix/dr_docs
Jul 29, 2022
Merged

clarify destination rule docs#2428
istio-testing merged 3 commits intoistio:masterfrom
ramaraochavali:fix/dr_docs

Conversation

@ramaraochavali
Copy link
Contributor

Clarify Destination Rule docs that Max Pending Requests/ Max Requests apply to both Http1 and Http2

See istio/istio#31548 and envoyproxy/envoy#9668 (See version history and doc updates)

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
@istio-testing istio-testing added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jul 23, 2022
@ramaraochavali ramaraochavali added release-notes-none Indicates a PR that does not require release notes. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jul 23, 2022
// Settings applicable to HTTP1.1/HTTP2/GRPC connections.
message HTTPSettings {
// Maximum number of pending HTTP requests to a destination. Default 2^32-1.
// Please note that this is applicable to both HTTP1.1/ and HTTP2.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HTTP/1.1 instead of HTTP1.1/ or leave the / out entirely as it's not in HTTP2.

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
@istio-testing istio-testing added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jul 25, 2022

// Maximum number of requests to a backend. Default 2^32-1.
// Please note that this is applicable to both HTTP/1.1 and HTTP2.
int32 http2_max_requests = 2;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so is it identical to http1_max_pending_requests then

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not exactly same. http2_max_requests is how many concurrent active requests can be there. max_pending_requests is how many requests can be in pending queue waiting for connection.

Specifically for pending requests,
"For HTTP/2 connections, if max concurrent streams and max requests per connection are not configured, all requests will be multiplexed over the same connection so this circuit breaker will only be hit when no connection is already established"

So there is subtle difference on how http2 protocol options are setup.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we clarify this difference in our api comments?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah. good point. Let me update it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated the docs. Defaults are also incorrect. Fixed them

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
@istio-testing istio-testing added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jul 27, 2022
@ramaraochavali
Copy link
Contributor Author

@howardjohn can you PTAL when you get chance?

@istio-testing istio-testing merged commit 8550728 into istio:master Jul 29, 2022
@ramaraochavali ramaraochavali deleted the fix/dr_docs branch July 30, 2022 04:18
frittentheke added a commit to frittentheke/istio that referenced this pull request Sep 19, 2025
…pplied to certain HTTP version

The circuit-breaker connection pool settings `MaxRequests`and `MaxPendingRequests` apply to HTTP/1.1
as well as HTTP/2 since [1] and [2].

This was actually changed in the Istio docs via [3], but these source code comments were left, potentially
causing confusion.

[1] envoyproxy/envoy#9215
[2] envoyproxy/envoy#9668
[3] istio/api#2428

Signed-off-by: Christian Rohmann <christian.rohmann@inovex.de>
frittentheke added a commit to frittentheke/istio that referenced this pull request Sep 19, 2025
…pplying to particular HTTP version

The circuit-breaker connection pool settings `MaxRequests`and `MaxPendingRequests` apply to HTTP/1.1
as well as HTTP/2 since [1] and [2].

This was actually changed in the Istio docs via [3], but these source code comments were left, potentially
causing confusion.

[1] envoyproxy/envoy#9215
[2] envoyproxy/envoy#9668
[3] istio/api#2428

Signed-off-by: Christian Rohmann <christian.rohmann@inovex.de>
istio-testing pushed a commit to istio/istio that referenced this pull request Sep 20, 2025
…pplying to particular HTTP version (#57705)

The circuit-breaker connection pool settings `MaxRequests`and `MaxPendingRequests` apply to HTTP/1.1
as well as HTTP/2 since [1] and [2].

This was actually changed in the Istio docs via [3], but these source code comments were left, potentially
causing confusion.

[1] envoyproxy/envoy#9215
[2] envoyproxy/envoy#9668
[3] istio/api#2428

Signed-off-by: Christian Rohmann <christian.rohmann@inovex.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release-notes-none Indicates a PR that does not require release notes. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants